Adding documentation and examples for Transparent Data Encryption, En… by mihymel · Pull Request #1260 · Azure/azure-rest-api-specs

mihymel · 2017-05-25T22:29:50Z

…cryption Protector, Server Key, and Servers

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

…cryption Protector, Server Key, and Servers

msftclas · 2017-05-25T22:29:54Z

@mihymel,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

dsgouda

Looks in pretty good shape. Left a few comments.

dsgouda · 2017-05-25T22:48:08Z

arm-sql/2015-05-01-preview/swagger/encryptionProtectors.json

+            "azurekeyvault",
+            "servicemanaged"
+          ],
+          "type": "string"


You'd want to consider setting "modelAsString":true if this enum is bound to change later, not doing so could result in breaking change. This applies to all other enums.

dsgouda · 2017-05-25T22:51:08Z

arm-sql/2015-05-01-preview/swagger/encryptionProtectors.json

+            "description": "The requested encryption protector resource state.",
+            "required": true,
+            "schema": {
+              "$ref": "#/definitions/EncryptionProtector"


Since EncryptionProtector is a body parameter here, please ensure you have set all the correct properties to readonly.

I double checked to be safe - ServerKeyType and ServerKeyName are the only properties of EncryptionProtector that are not readonly. This is what we want.

dsgouda · 2017-05-25T22:54:44Z

arm-sql/2015-05-01-preview/swagger/encryptionProtectors.json

+  "produces": [
+    "application/json"
+  ],
+  "paths": {


Please ensure there is an Operations API that exposes information about the operations exposed here.

It's in sql.core.json

dsgouda · 2017-05-25T22:55:54Z

arm-sql/2015-05-01-preview/swagger/encryptionProtectors.json

+          "EncryptionProtectors"
+        ],
+        "description": "Returns a list of the server encryption protectors",
+        "operationId": "Servers_ListEncryptionProtectors",


You probably mean Servers_ListByEncryptionProtectors

EncryptionProtector is a singleton, so this is following the pattern for naming singleton operations. See blob auditing policies for example of the pattern.

dsgouda · 2017-05-25T23:07:00Z

arm-sql/2015-05-01-preview/swagger/serverKeys.json

+          ],
+          "type": "string"
+        },
+        "location": {


Just to confirm, by adding location you dont intend to make this a tracked resource.

Correct. This is not a tracked resource.

dsgouda · 2017-05-25T23:37:50Z

arm-sql/2015-05-01-preview/swagger/sql.core.json

+          "Servers"
+        ],
+        "description": "Creates or updates a server.",
+        "operationId": "Servers_CreateOrUpdate",


If you already have an update operation you probably don't need a createOrUpdate

"CreateOrUpdate" is for PUT, "Update" is for PATCH.

Let me rephrase that, do you think you will need a CreateOrUpdate even if you had Create and Update operations?
CreateOrUpdate is useful if the update operation only allows an update on the tags property. If you wish to expose a PATCH operation that allows updating of other properties, its better to have a separate Update operation like you have here.

There is no Create operation. Just Servers_CreateOrUpdate and Servers_Update

dsgouda · 2017-05-25T23:38:41Z

arm-sql/2015-05-01-preview/swagger/sql.core.json

+          }
+        },
+        "x-ms-long-running-operation": true
+      }


Please provide x-ms-examples here.

dsgouda · 2017-05-25T23:41:16Z

arm-sql/2015-05-01-preview/swagger/sql.core.json

+        },
+        "kind": {
+          "description": "Kind of sql server. This is metadata used for the Azure portal experience.",
+          "enum": [


please provide an x-ms-enums section with appropriate options here,

dsgouda · 2017-05-26T18:03:03Z

arm-sql/2015-05-01-preview/swagger/encryptionProtectors.json

+        "operationId": "Servers_CreateOrUpdateEncryptionProtector",
+        "parameters": [
+          {
+            "$ref": "#/parameters/ResourceGroupParameter"


Maintain consistent parameter ordering, either place "ResourceGroupParameter" and "ApiVersionParameter" together at the beginning or at the end of the parameters section.

Why is this? You previously told us that SubscriptionId and ApiVersion should be together since they are client params, not method params. Did this change?

Whoops I confused SubscriptionId with ResourceGroupName, sorry about that, please disregard this

…tring for enum parameters, fixing sql.core name to servers, adding x-ms-examples to servers

jaredmoo · 2017-05-26T20:18:30Z

@nathannfan to review

nathannfan · 2017-05-26T21:06:51Z

arm-sql/2015-05-01-preview/swagger/encryptionProtectors.json

+            ],
+            "x-ms-enum": {
+              "name": "EncryptionProtectorName",
+              "modelAsString": true


I assume this might change?

EncryptionProtector is a singleton now and we intend for it to be that way for the unforeseeable future. Technically (with some changes) the API is designed to support multiple EncryptionProtectors, so I used modelAsString.

…odelAsEnum" for some string properties due to resulting errors in generated .net SDK

mihymel · 2017-05-26T21:31:28Z

I removed "x-ms-enum" for some parameters where an acceptable enum value included an empty string because in this case this resulted in errors in the generated .NET SDK. The generated .NET SDK generates an enum class where the enum with value "" doesn't have a name: e.g. public const string = "";

nathannfan · 2017-05-26T21:33:43Z

@dsgouda the validation error for syntax isn't very clear. Is this a legitimate syntax error in the swagger?

dsgouda · 2017-05-26T21:39:33Z

If there are no explicit properties you wish to add under "x-ms-enums", you can remove it or just put an empty object there, I leave it to your discretion.

dsgouda · 2017-05-26T21:40:10Z

@nathannfan which specific error is this related too, I can try and help you out with it.

nathannfan · 2017-05-26T21:41:31Z

@dsgouda thanks, I'm referring to https://travis-ci.org/Azure/azure-rest-api-specs/jobs/236527345#L1056

…r api 2014-04-01 so they can be added to compositeSql

AutorestCI · 2017-05-27T00:21:33Z

No modification for NodeJS

AutorestCI · 2017-05-27T00:21:40Z

Azure/azure-sdk-for-ruby#790

AutorestCI · 2017-05-27T00:22:05Z

Azure/azure-sdk-for-python#1198

Adding documentation and examples for Transparent Data Encryption, En…

12bc96a

…cryption Protector, Server Key, and Servers

msftclas added the cla-not-required label May 25, 2017

azuresdkci assigned dsgouda May 25, 2017

dsgouda suggested changes May 25, 2017

View reviewed changes

dsgouda reviewed May 26, 2017

View reviewed changes

Addressing comments from pull request. Adding x-ms-enums and modelAsS…

6d91fa2

…tring for enum parameters, fixing sql.core name to servers, adding x-ms-examples to servers

nathannfan approved these changes May 26, 2017

View reviewed changes

Updating server update examples to fix validation errors. Removing "m…

376a6bb

…odelAsEnum" for some string properties due to resulting errors in generated .net SDK

Pulling out usages and serviceObjectives from servers swagger file fo…

0e46535

…r api 2014-04-01 so they can be added to compositeSql

jaredmoo approved these changes May 27, 2017

View reviewed changes

dsgouda approved these changes May 27, 2017

View reviewed changes

dsgouda merged commit 76ef41d into Azure:master May 27, 2017

AutorestCI mentioned this pull request May 27, 2017

Automatic PR from RestAPI-PR1260 Azure/azure-sdk-for-ruby#790

Merged

AutorestCI mentioned this pull request May 27, 2017

[SQL] Automatic PR from RestAPI-PR1260 Azure/azure-sdk-for-python#1198

Closed

jaredmoo mentioned this pull request Jun 7, 2017

SQL server AD identity and data encryption support Azure/azure-sdk-for-net#3334

Merged

8 tasks

Conversation

mihymel commented May 25, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

msftclas commented May 25, 2017

Uh oh!

dsgouda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsgouda May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredmoo commented May 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihymel commented May 26, 2017

Uh oh!

nathannfan commented May 26, 2017

Uh oh!

dsgouda commented May 26, 2017

Uh oh!

dsgouda commented May 26, 2017

Uh oh!

nathannfan commented May 26, 2017

Uh oh!

AutorestCI commented May 27, 2017

Uh oh!

AutorestCI commented May 27, 2017

Uh oh!

AutorestCI commented May 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mihymel commented May 25, 2017 •

edited by azuresdkci

Loading

dsgouda May 26, 2017 •

edited

Loading